Move TextMatcher from Platform UI to Equinox Common #720
Conversation
|
Prerequisite to eclipse-platform/eclipse.platform.ui#2567 |
|
@tjwatson any concerns here? |
|
I'm really not sure if this is the right place. Equinox common contains code related to the runtime (in the broadest sense). |
The main motivator for all of this are plugin dependencies. Several UI plugins use the TextMatcher and to prevent having to add any new dependencies, I have to find the a common denominator, which is But none of the re-exported plugins really have anything to do with text handling. So the reason I picked Equinox is, as you noticed, the StringMatcher, because that's at least somewhat related. |
|
|
The API signature does not reference
It seems in the spirit of https://bugs.eclipse.org/bugs/show_bug.cgi?id=508611 so I don't have an issue with the functionality existing in this package of Equinox common. But TextMatcher vs. StringMatcher have arbitrary names that are confusing to one that has never used either of these classes. It seems overkill to add a new slightly differently named class just to add the following public APIs, one which is a static method that could go anywhere: /**
* Determines whether the given {@code text} matches the pattern.
*
* @param text String to match; must not be {@code null}
* @return {@code true} if the whole {@code text} matches the pattern;
* {@code false} otherwise
* @throws IllegalArgumentException if {@code text == null}
*/
public boolean match(String text)
/**
* Determines whether the given sub-string of {@code text} from {@code start}
* (inclusive) to {@code end} (exclusive) matches the pattern.
*
* @param text String to match in; must not be {@code null}
* @param start start index (inclusive) within {@code text} of the sub-string to
* match
* @param end end index (exclusive) within {@code text} of the sub-string to
* match
* @return {@code true} if the given slice of {@code text} matches the pattern;
* {@code false} otherwise
* @throws IllegalArgumentException if {@code text == null}
*/
public boolean match(String text, int start, int end)
/**
* Splits a given text into words.
*
* @param text to split
* @return the words of the text
*/
public static String[] getWords(String text) {I think it would be better to enhance |
ptziegler
force-pushed
the
text-matcher
branch
2 times, most recently
from
f678abb to
8ff10ad
Compare
bundles/org.eclipse.equinox.common/src/org/eclipse/core/text/StringMatcher.java
Outdated
Show resolved
Hide resolved
ptziegler
force-pushed
the
text-matcher
branch
from
8ff10ad to
f14e97a
Compare
That makes sense. Luckily, the TextMatcher effectively extends the StringMatcher, so it was fairly easy to add this new functionality. |
ptziegler
force-pushed
the
text-matcher
branch
3 times, most recently
from
89f7d99 to
4874832
Compare
| fLength = pattern.length(); | ||
|
|
||
| fIgnoreWords = ignoreWords; | ||
| if (fIgnoreWords) { |
There was a problem hiding this comment.
With only my API hat on I think it would make more sense if we didn't add a new constructor to take an additional boolean for word matching. Instead could we add a new method public boolean matchWords(List<String> words) or public boolean matchWords(String[] words)
I'm unsure how much that complicates the implementation. I would imagine you would want to use an AtomicReference<fParts[]> to create the parts on demand if/when matchWords is called.
There was a problem hiding this comment.
Instead could we add a new method public boolean matchWords(List words) or public boolean matchWords(String[] words)
I suppose that would work. Though I think the method has to accept the whole text, given that the sub-patterns are matched against the text as well as the individual words. I can't really predict the impact if I were to change this behavior.
I would imagine you would want to use an AtomicReference<fParts[]> to create the parts on demand if/when matchWords is called.
I'm not dead-set on the final modifier, so an AtomicReference sounds like overkill if I can do a lazy initialization by checking whether fParts == null.
I'm unsure how much that complicates the implementation.
It'll primarly make the check more verbose. So instead of matcher.match(text) it'll become matcher.match(text) || matcher.matchWords(text). There are 4-5 Platform classes that need to be updated, so I think that's a doable amount of work.
It obviously also means that the trimming of the pattern has to be done explicitly, but that's also fine.
There was a problem hiding this comment.
It'll primarly make the check more verbose. So instead of matcher.match(text) it'll become matcher.match(text) || matcher.matchWords(text). There are 4-5 Platform classes that need to be updated, so I think that's a doable amount of work.
Alternatively, I can also simply call match(text) inside matchWords(text) which I think is preferable.
There was a problem hiding this comment.
It'll primarly make the check more verbose. So instead of matcher.match(text) it'll become matcher.match(text) || matcher.matchWords(text). There are 4-5 Platform classes that need to be updated, so I think that's a doable amount of work.
Alternatively, I can also simply call
match(text)insidematchWords(text)which I think is preferable.
That was my intention, make the new method do what you wanted to do with the existing method.
ptziegler
force-pushed
the
text-matcher
branch
from
d8e3267 to
decd4ed
Compare
The TextMatcher class is used in the UI component, despite not depending on any UI classes. By moving it to Equinox, it can be used anywhere in the Eclipse Platform.
ptziegler
force-pushed
the
text-matcher
branch
from
decd4ed to
66d35bb
Compare
The TextMatcher class is used in the UI component, despite not depending on any UI classes. By moving it to Equinox, it can be used anywhere in the Eclipse Platform.